Cache C# Compilation Emit by Project Version to avoid excessive background churn - #20119
Cache C# Compilation Emit by Project Version to avoid excessive background churn#20119xperiandri wants to merge 3 commits into
Conversation
- Add ActiveDocumentDetection module (IVsMonitorSelection-based helper) - Gate UnusedDeclarationsAnalyzer to active document - Gate SimplifyNameDiagnosticAnalyzer to active document - Gate FSharpInlayHintsService to active document - Gate UnusedOpensDiagnosticAnalyzer to active document - Add ActiveDocumentDetection.fs to FSharp.Editor.fsproj Fixes dotnet#20114
github-actions
Bot
added
⚠️ Affects-Agent-Config
|
🔍 Tooling Safety Check — Affects-Build-Infra, Affects-Restore, Affects-Agent-Config, Scope-Review-Needed
|
T-Gro
left a comment
T-Gro
left a comment
There was a problem hiding this comment.
🤖 This review was generated by AI (@expert-reviewer agent). Findings may contain inaccuracies — please verify independently.
Review scoped to the substantive behavioural change in FSharpProjectOptionsManager.fs (the new C# emit cache). The remaining diff is largely an arcade/eng/common and net11→net10 TargetFramework revert plus analyzer active-document gating, which were not reviewed in depth. Three correctness/performance concerns on the cache are noted inline; the most important is the unbounded growth of the per-project version cache, which can turn the intended optimization into a memory leak under the very churn scenario it targets.
| // However, when C# projects churn, Roslyn creates new Compilation instances with the same project ID and version, | ||
| // which makes ConditionalWeakTable defeat the purpose. We use a nested ConcurrentDictionary keyed by ProjectId and VersionStamp | ||
| // to map to the FSharpReferencedProject, ensuring stable references across churns. | ||
| let emitCache = ConcurrentDictionary<ProjectId, ConcurrentDictionary<VersionStamp, FSharpReferencedProject>>() |
There was a problem hiding this comment.
Unbounded memory growth (potential leak). The inner versionCache is only pruned when the whole project is removed from the solution (emitCache.TryRemove(projectId)); individual VersionStamp entries are never evicted. Each entry strongly holds an FSharpReferencedProject whose DelayedILModuleReader retains the emitted metadata MemoryStream once realized (the code deliberately never disposes it). Under exactly the C#-churn scenario this PR targets, every edit yields a new stamp and appends a new entry, so this dictionary grows without bound for the life of the project. This replaces the previous GC-collectable ConditionalWeakTable<Compilation,_> (entries freed once the Compilation was collected) with a strongly-rooted cache. Consider keeping only the latest stamp per project (clear/replace on a new stamp) or bounding the cache size (LRU).
| let createPEReference (referencedProject: Project) (comp: Compilation) ct = | ||
| cancellableTask { | ||
| let projectId = referencedProject.Id | ||
| let! stamp = referencedProject.GetDependentVersionAsync(ct) |
There was a problem hiding this comment.
GetDependentVersionAsync changes on any text edit to this project or to any project it transitively references, so the cache will miss — and trigger a fresh, expensive metadata Emit — far more often than needed, and (combined with the unbounded versionCache above) accumulates entries faster. For a metadata-only PE reference the meaningful key is the semantic version: GetDependentSemanticVersionAsync changes only when the referenced project's public surface changes, which is what actually invalidates the emitted metadata. Also, the PR description states the key is project.Version, which does not match this call — please reconcile description and implementation.
| weakPEReferences.Add(comp, fsRefProj) | ||
| versionCache.[stamp] <- fsRefProj | ||
| return fsRefProj | ||
| | _ -> |
There was a problem hiding this comment.
This first-seen initialization path races: two threads hitting a not-yet-cached projectId each construct a separate versionCache, and emitCache.[projectId] <- versionCache unconditionally overwrites, discarding the other thread's just-added entry and forcing a redundant Emit. Use let versionCache = emitCache.GetOrAdd(projectId, fun _ -> ConcurrentDictionary<_,_>()) and then follow a single code path. That also lets you delete the ~55 lines of getStream/tryStream logic duplicated verbatim between this branch (lines 218-274) and the branch above (lines 153-209); keeping two identical copies risks them silently diverging when one is later fixed and the other is missed.
3 participants
Fixes #20118.
Description
When C# projects change, Roslyn creates new
Compilationinstances. The F# IDE integration currently uses aConditionalWeakTableto cache the emitted PE reference from these compilations. Because newCompilationobjects are continually created by Roslyn, the weak table cache misses, causing repeated, expensive metadata-only emissions (Compilation.Emit(metadataOnly=true)). This contributes to UI latency and background CPU churn.Solution
This PR adds an
emitCache: ConcurrentDictionary<ProjectId, ConcurrentDictionary<VersionStamp, FSharpReferencedProject>>toFSharpProjectOptionsManager.fs.This allows caching based on a stable identifier (
projectIdandproject.Version), avoiding unnecessary re-emission of assemblies when the underlying C# project hasn't functionally changed. The cache properly invalidates and handlesCancellationTokenviacancellableTask { ... }.